Skip to content

feat(models): OpenAI-compatible /v1/* gateway as built-in Resources (#631, Phase 4 of #510) - #1616

Open
heskew wants to merge 43 commits into
mainfrom
feat/models-v1-gateway
Open

feat(models): OpenAI-compatible /v1/* gateway as built-in Resources (#631, Phase 4 of #510)#1616
heskew wants to merge 43 commits into
mainfrom
feat/models-v1-gateway

Conversation

@heskew

@heskew heskew commented Jul 6, 2026

Copy link
Copy Markdown
Member

Summary

OpenAI-compatible /v1/* gateway as built-in core Resources (Phase 4 of #510): POST /v1/embeddings, POST /v1/chat/completions (streaming + non-streaming), and GET /v1/models — thin protocol-translation wrappers over scope.models. Unmodified OpenAI SDK / LangChain.js clients point baseURL at Harper's REST port and work.

⚠️ Where to look first — transformIterable fix (core serving path)

server/serverHelpers/contentTypes.ts transformIterable() called transform(step.value) on the terminal {done: true, value: undefined} step, so the text/event-stream serializer threw at the end of any finite async-iterable response body (latent until now: existing SSE is infinite subscriptions closed via .return()). Minimal guard, both sync and async paths; covered by a real-HTTP + real-eventsource unit test through the full serializeStream path.

Known CI caveat — integration test flake until #1618 lands

integrationTests/server/v1-gateway.test.ts passes or fails as a unit, run-to-run, on identical code (~50% on Linux CI; macOS reliably passes). Root cause was pinned via instrumented CI runs and is NOT in this PR: scope.options (OptionsWatcher) re-parses the on-disk config file, which races the env-var config flush at boot — so handleApplication's enabled check can read pre-env values even though componentLoader itself saw enabled: true (probe-verified on both threads of a failing run). The fix is up as the #1618 PR (OptionsWatcher composes env layers over root-config reads); once it merges this test is deterministic. Instrumentation commits were reverted (58bb6e867/2f000ea13 + reverts in history).

Resolved in review (cross-model, adjudicated)

Body-await on POST handlers (request.data is a Promise on the REST path — the integration test's 200 assertions had been masked by the CI 404 and had never actually validated); auth enforcement (above); model-id conflation (fixed upstream by #1596, with tests); 5xx message hygiene; 3 gemini inline suggestions applied earlier. The AbortSignal PR-body claim was verified TRUE by the adjudicator (ALS capture at Models.ts:151,202).

Open for review

  • Permission granularity: super_user (default-Resource parity) — deliberately conservative; loosen to a dedicated permission if desired.
  • The transformIterable guard is the one change outside the new module.

Docs

Companion PR: HarperFast/documentation#568 (synced to the enabled: true form and the 401/super_user auth behavior).

Closes #631
Tracking: #510


Generated by LLMs (Claude Sonnet 5 implementation workers + Claude Fable 5 coordination/diagnosis, via Claude Code). Cross-model reviewed: Codex leg + Harper-domain adjudication (Gemini leg failed on tooling that day — no second-model perf coverage; noted per skill protocol).

🤖 Generated with Claude Code

REST activation on bare instances (added 2026-07-17)

The gateway's resources are served by REST's middleware chain, which only activates when a component config contains a rest/REST section — a bare instance (no deployed apps) has none, leaving the endpoints registered but unservable (this was the persistent integration-test 404; never a config race). Fix: componentLoader calls the new REST.ensureStarted({ server, resources }) after the root plugin loop completes, gated on modelsGateway.enabled. Ordering is safe by construction: any user rest/REST section has already registered with its own options by that point, making the call a no-op (verified live: rest: { webSocket: false } alongside the gateway keeps the ws chain REST-free and registers the http handler exactly once). /v1/models also now dedupes logical names registered for both generative and embedding, and the streaming integration test authenticates the OpenAI SDK with a real operation token — the documented production flow (Harper JWT as the api key).


Rework (2026-07-24): clean-plugin refactor + @kriszyp review

⚠️ Supersedes the "REST activation on bare instances" section above. That approach (a REST.ensureStarted() export + a modelsGateway-named branch in componentLoader) has been removed — it force-started REST before application configs loaded, which silently discarded an app's own rest options (webSocket, urlPath/host, middleware ordering). @kriszyp caught this.

The gateway is now a clean plugin

Core footprint dropped from a named special-case + an invented core API to a single registry entry:

  • server/REST.ts is byte-identical to main — the exported ensureStarted() that existed only for this feature is gone.
  • components/componentLoader.ts no longer names this feature; the registry entry is the lazy string specifier #src/resources/models/v1/index (the loader await import()s string plugin values; #src/* resolves under both --conditions=typestrip and dist).
  • The gateway requires a rest config section and documents it, rather than forcing REST to start. handleApplication warns if enabled without rest. The integration test declares rest as a real deployment would.

Core gaps are tracked, not worked around: #1931 (a supported way for a component to declare "I serve REST resources", including the after-app-load ordering) and #1932 (a per-/v1 auth-error-shaping hook so invalid-token 401s get the OpenAI envelope — the one review thread left open as a deliberate follow-up).

Review findings addressed (all in-PR unless noted)

  • response_format: json_schema — extract .schema from the {name,strict,schema} wrapper (was double-wrapping); test corrected to the real wire shape.
  • Malformed nested wire shapes (messages:[null], tool_calls:[{}], tools:[{}], non-array tools, un-flattenable content parts) and rejected body promises now return OpenAI 400s instead of escaping as RFC 9457 500s — shared validateChatRequest, applied to both chat and embeddings.
  • tool_choice: 'none' omits tools entirely; 'required'/named return 400 rather than being silently downgraded to auto.
  • openaiStream tool assembly: Object.assign (null-prototype) instead of O(n²) re-spread, with per-stream call-count / argument-field bounds terminating through the sanitized SSE error frame.
  • OpenAI content parts (content: [{type:'text',...}]) flattened to the string Message.content expects; unsupported part types 400.
  • Explicit Accept: text/event-stream clients are served via a new connect() delegating to post() (previously hit Resource's default → a forever-open empty subscription).

Cross-model review (HEG step 10, thorough)

Codex + Harper-domain adjudication. Caveat: the Gemini leg was unavailable (agy model/location config error) — no outside-model perf/duplication lens this round. The review caught real defects that are now fixed: a __proto__ tool-arg field silently dropped by the Object.assign switch (null-prototype accumulator + regression test); the typestrip-resolution regression (→ the #src string specifier); the /v1/embeddings symmetry miss; the broken doc example; the content-parts gap; and that the O(n²) bound-check hadn't actually been removed. CI additionally caught an unpassable assertion in a new integration test (the fixture streams word-by-word, so the content never appears contiguously in raw SSE — now reassembles deltas like a client). 385 unit tests, CI green.

Also merged current main in (picks up #1853's DatabaseTransaction.ts type fix, which had been failing the Next.js adapter build).


Round-3 @kriszyp review (2026-07-31): all 9 findings fixed

Ten commits, bc31ac75e..6cdc4010c, one per finding (threads replied + resolved with SHAs):

  • package-lock (bc31ac75e): restored production-native metadata my earlier npm-10 regen damaged — @msgpackr-extract/* dev flags healed, rocksdb libc selectors restored verbatim from main. Verified in a pristine node:24.18.0-bookworm container: one libc variant selected, msgpackr survives prune-shrinkwrap-dev, lock byte-stable. ⚠️ Upstream footgun flagged in-thread: the registry's abbreviated packument omits libc, so the next rocksdb bump on main will silently re-drop these selectors unless guarded.
  • Config-block removal (cfb3b5cd9): fixed generically in ScopeOptionsWatcher's remove now requests a restart like change, with the same defer-to-plugin convention, so deleting modelsGateway (the canonical disabled state) actually stops the gateway.
  • Route reservation (f43b7acfb): static reservedPath = true on the three gateway classes; Resources.set treats differing-class registration against a reserved path as a conflict (ErrorResource + logged error). Unit + collision-fixture integration tests; live-verified via direct spawn.
  • Protocol visibility (75da15071): explicit exportTypes — REST all three, SSE chat-only, ws/mqtt/graphql/mcp false. Writing the routing assertions exposed a latent core bug: getMatch's exact-path/root fallbacks returned protocol-blocked entries (MCP's isMcpExposed re-check was masking it). Blocked entries now fall through as not-found; every getMatch caller traced and unaffected-or-corrected.
  • Top-level validation (32016eed5): non-string model (chat + embeddings), non-boolean stream, out-of-range temperature, non-positive-integer max_tokens/max_completion_tokens → OpenAI 400s.
  • Capability mismatches (ca162e923): ModelCapabilityError400 invalid_request_error / capability_unsupported on both JSON and SSE paths (was a sanitized 500).
  • developer role (37b33598c): normalized to internal system (OpenAI treats the two identically; every adapter maps system to its system-instruction slot — priority preserved without widening the internal Message union). Unknown roles → 400. Covered on pure-translation and composed gateway→AnthropicBackend paths.
  • Embeddings usage (edccb0d0b): internal Models.embedWithUsage() (public embed() unchanged, delegates); gateway reports the backend's real usage with embeddingTokens in both prompt_tokens and total_tokens. Nonzero end-to-end test via TestBackend.
  • Stream assembly memory bound (88af2574f + 6cdc4010c): cumulative per-stream serialized budget (1,048,576 chars) over ids, names, and every argument value, charged per delta and monotonically (replacements included); follow-up charges per-entry JSON syntax and per-call envelope so the count upper-bounds the real serialization. Overflow → sanitized SSE error frame. Tests: oversized single value, tiny-entry syntax flood, replacement churn, under-budget completion.

Cross-model review (HEG, thorough): Codex leg + Harper-domain adjudication. The Codex leg caught the budget syntax under-count (fixed, 6cdc4010c). Caveats: the Codex CLI timed out mid-run (courier-completed — treated as low-assurance), and the Gemini/agy leg remains down ("model not supported in location"), so the domain pass is the primary signal: it traced the Resources.set/getMatch semantic changes to every consumer, the Scope remove lifecycle, and the embedWithUsage accounting — no blockers, no significant concerns.

Local suites: resources 1364✓ (one pre-existing audit-log flake, fails identically on a stash control without these changes), components/mcp/models sweeps green.

heskew and others added 2 commits July 6, 2026 08:40
Registers three REST resources on the REST port when `modelsGateway: {}`
is present in config:
  POST /v1/chat/completions  — streaming and non-streaming chat
  POST /v1/embeddings        — embedding endpoint
  GET  /v1/models            — enumerate registered backends

Key design points:
- OpenAI SDK sends `Accept: application/json` for ALL requests (incl.
  streaming). Harper's REST layer only dispatches `Accept: text/event-stream`
  as CONNECT; everything else is dispatched as the HTTP method. So
  `stream: true` chat requests land in `post()`, not `connect()`.
  The resource returns `{ body: Readable }` which REST.ts bypasses
  serialisation on (REST.ts:165-193).
- Fixes a latent bug in `transformIterable` (contentTypes.ts): the
  transform function was called on the terminal `{ done: true }` step of
  async generators, crashing when `serialize()` received `undefined`.
  Guard added: skip transform on any `done: true` step.
- Pure shape-mapper layer in `v1/translation.ts`; all functions are
  side-effect-free and fully unit-tested without a running server.
- OpenAI error envelope (`{ error: { message, type, code, param } }`)
  built in `v1/errors.ts`; resources catch errors themselves to avoid
  REST.ts serialising them as RFC 9457 Problem Details.
- `tool_choice: 'auto'` maps to `toolMode: 'return'`; full in-process
  orchestration is #612 (out of scope for this PR).
- `openai` added as devDependency (^6.45.0) for future integration tests.
- `listBackends(kind)` added to backendRegistry.ts for GET /v1/models.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…#631)

Adds an integration test that starts a real Harper instance with:
- `modelsGateway: {}` in config
- A deterministic echo backend registered via the registerFromModule path
  (CJS fixture at integrationTests/server/fixtures/v1-gateway-test-backend.cjs)

Tests all three endpoints:
- GET /v1/models — model list shape
- POST /v1/embeddings — single and batched input; 400 error shape
- POST /v1/chat/completions — non-streaming shape; 400 error shape;
  streaming via the real OpenAI Node.js SDK (validates full SSE framing)

The streaming test specifically exercises the SSE serving-path: the OpenAI
SDK sends Accept: application/json for all requests, so stream: true lands
in post() not connect(). The resource returns { body: Readable } which
REST.ts bypasses serialisation on, and the SDK successfully parses the
[DONE]-terminated SSE stream.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@socket-security

socket-security Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedopenai@​6.45.078100100100100

View full report

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an OpenAI-compatible '/v1/*' REST gateway for Harper, adding endpoints for listing models, generating embeddings, and handling both streaming and non-streaming chat completions. It also resolves a critical bug in 'transformIterable' that caused crashes during the terminal step of streaming responses. Feedback from the review highlights opportunities to propagate the client's 'AbortSignal' to save server resources on disconnects, strengthen request body validation (such as checking for arrays in the embeddings endpoint and validating message structures), and defensively handle tool call arguments to prevent double-serialization.

Comment thread resources/models/v1/chatCompletions.ts Outdated
Comment thread resources/models/v1/embeddings.ts Outdated
Comment thread resources/models/v1/translation.ts
Comment thread resources/models/v1/chatCompletions.ts Outdated
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@heskew
heskew requested a review from kriszyp July 6, 2026 17:21
heskew and others added 2 commits July 6, 2026 10:24
npm ci in CI was failing with "Missing: openai@6.45.0 from lock file"
because package-lock.json was not updated when openai was added to
devDependencies. Ran `npm install openai --package-lock-only`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use `modelsGateway: { enabled: true }` in the integration test; an empty
  object `{}` is silently dropped by `flattenObject()` in
  `harperConfigEnvVars.ts` (no leaf paths → nothing set via HARPER_SET_CONFIG)
  so `componentLoader` never sees the key and skips loading the gateway
- Add `|| Array.isArray(body)` guard to `V1Embeddings.post` (Gemini: arrays
  pass `typeof x !== 'object'` unchanged since `typeof [] === 'object'`)
- Guard against pre-serialised string arguments in `toOAIToolCalls` (Gemini:
  passing a JSON string through `JSON.stringify` would double-encode it)
- Rename inner `body` → `readable` in `V1ChatCompletions.post` streaming path
  (Claude bot: shadowed the outer `body` parameter at line 28)
- AbortSignal thread left as informational: signal is already propagated via
  `resolveCallContext` / `contextStorage.getStore()?.signal` in `Models.ts`

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
heskew and others added 2 commits July 6, 2026 11:51
server/REST.ts builds `request.data` via the streaming JSON deserializer
and hands it to resource methods unawaited, so the `body` argument
V1ChatCompletions.post/V1Embeddings.post received was a Promise, not the
parsed object. Every real JSON POST to the gateway was reading undefined
fields off a Promise and returning 400 — the integration test's 200
assertions were masked by the (separate) 404 gating bug and had never
actually exercised this path. Await the body before reading any field,
including `stream`.

Adjudicated cross-model review finding on #1616.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Static method overrides on V1Models, V1ChatCompletions, and V1Embeddings
bypass Resource's transactional() wrapper and therefore the default
allowRead/allowCreate super_user gate.

Add authorizeV1Request() in errors.ts that returns a well-formed OpenAI
error envelope (401 for anonymous, 403 for non-super_user, null to
proceed). Call it at the top of each handler before touching the body.

Unit tests cover: 401 anon, 403 non-super_user, 200 super_user, and
that auth is checked before body validation (prevents body deserialization
on unauthorized requests).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread resources/models/v1/index.ts Outdated
heskew and others added 2 commits July 6, 2026 16:26
…bled flag

Previously the gateway only loaded if the user added a `modelsGateway:` key
to harperdb-config.yaml (presence-gating). On CI and fast-startup environments
the HARPER_SET_CONFIG env var raced component loading, causing intermittent 404s.

Add `modelsGateway: { enabled: false }` to defaultConfig.yaml so the key is
always present in the resolved config and componentLoader always sees it. The
`handleApplication` entry point now checks `scope.options.get(['enabled'])` and
returns immediately when false (same pattern as the agent component).

Opt in with `modelsGateway: { enabled: true }`. The integration test already
passes this explicitly; its new header comment notes the dependency on #1618
(env-config ordering) for reliable CI behaviour.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread resources/models/v1/errors.ts
heskew and others added 2 commits July 8, 2026 15:01
CI's Format Check runs the lockfile prettier (3.9.3); the files were formatted
with 3.8.3 which disagrees on these three.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…— REVERT BEFORE MERGE

Logs per-thread: componentLoader's resolved modelsGateway config + env-var presence,
handleApplication's enabled value, and (test-side) the child's harper-config.yaml
block + get_configuration.modelsGateway.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread resources/models/v1/index.ts Outdated
Comment thread components/componentLoader.ts Outdated
heskew and others added 3 commits July 8, 2026 15:12
Internal error strings (backend stack details, paths) don't belong in the wire
response. 4xx messages are client-actionable and pass through unchanged.
Adjudicated cross-model review suggestion on #1616.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) — REVERT BEFORE MERGE

Probes the install/config path this time: createConfigFile post-env-apply +
post-validate, updateConfigValue entry + pre-write, per-thread
loadRootComponents config view, plus the round-1 loader/handler probes.
Test-side dump intentionally NOT restored (round 1's before() awaits delayed
the first request and masked the race — that run went green).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread resources/models/v1/index.ts Outdated
… 404 (#1616) — REVERT BEFORE MERGE"

This reverts commit 2f000ea.
Comment thread resources/models/v1/errors.ts
401 = bad/missing credentials (authentication_error); 403 = valid credentials
lacking permission (permission_error) — matching authorizeV1Request's own
envelope. Addresses claude-bot review feedback on #1616.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
heskew added a commit that referenced this pull request Jul 8, 2026
…ush (#1618)

OptionsWatcher re-parsed the on-disk root config, while componentLoader decides
from the resolved in-memory config — which deterministically includes runtime
env config (HARPER_SET_CONFIG et al.). At boot the env-applied values reach the
file only after the first thread's runtime re-apply flushes them, so a
component's boot-time scope.options reads (e.g. an enabled gate in
handleApplication) could observe pre-env values the loader never saw. Proven
via instrumented CI runs on #1616: componentLoader logged enabled:true on every
thread of a failing run while the routes 404'd.

OptionsWatcher now composes the env layers (composeConfigFromEnv, side-effect
free, same precedence as the runtime pipeline) over every root-config read,
including the install window where the file does not exist yet. Application
config.yaml scopes are untouched, as is behavior when no config env vars are
set. flattenObject additionally warns once per path when an env-var config
value is an empty object — it flattens to nothing by design (load-bearing for
restore-on-removal semantics) but the silent drop has confused users.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread integrationTests/server/v1-gateway.test.ts Outdated
…ly, serve explicit SSE

Addresses the remaining plugin-local findings from @kriszyp's review on #1616.

- response_format json_schema: the wire value is a `{ name, strict, schema }`
  wrapper, but Harper's contract wants the schema itself. Passing the wrapper
  made the backend wrap it again and send metadata where the provider expects
  the JSON Schema. Now extracted, and the unit test uses the real wrapper shape.
- Malformed nested wire shapes (`messages:[null]`, `tool_calls:[{}]`,
  `tools:[{}]`, non-array `tools`) threw TypeErrors inside the mappers, escaping
  as RFC 9457 500s. A shared `validateChatRequest` now rejects them with an
  OpenAI 400 before mapping; the mapping moved inside the error boundary, and a
  rejected body promise (malformed JSON) is a 400 rather than a 500.
- tool_choice was advertised but never read: `none`, `required` and named
  selection all behaved as `auto` with every tool still forwarded. `'none'` now
  omits tools entirely, and choices the internal contract cannot represent
  return a clear 400 instead of being silently downgraded.
- openaiStream tool assembly re-spread every accumulated argument on each
  partial delta (O(n²)) and grew `toolAssembly` without a cap on a public HTTP
  path. Now mutates via Object.assign with per-stream call-count and
  argument-field bounds, terminating through the sanitized error-frame path.
- A client sending an explicit `Accept: text/event-stream` is dispatched by REST
  as CONNECT, which this Resource did not implement — so valid SSE clients got
  method-not-allowed (the OpenAI SDK only escapes this by always sending
  application/json). `connect()` now delegates to the same `post()`
  implementation, recovering the body REST passes as null.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
Comment thread resources/models/openaiStream.ts
… deltas in test

Two follow-ups from cross-model review and CI on the previous commit.

- The switch from spread to Object.assign (to kill the O(n^2) re-copy) changed
  the write semantics: spread uses CreateDataProperty, Object.assign uses
  [[Set]]. Tool arguments come from JSON.parse, where a field literally named
  `__proto__` is an own property — so on an ordinary object Object.assign hit
  Object.prototype's inherited setter and silently dropped it. Verified: an
  ordinary accumulator yields {"safe":1}, a null-prototype one preserves the
  field. Accumulator is now Object.create(null), with a regression test.
  (Caught by the Codex leg.)

- The explicit-SSE integration assertion could never pass: the fixture streams
  word by word, so `[echo stream]` is split across `data:` frames and never
  appears contiguously in the raw text. It now parses the frames and
  reassembles the deltas the way a client does. The endpoint itself was
  correct — verified against a live instance. (Caught by CI.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
Comment thread components/componentLoader.ts Outdated
…y, content parts

Follow-ups from the thorough review pass on the rework.

- Registry entry is now the string `#src/resources/models/v1/index` rather than a
  lazy require() getter. The loader already await-import()s string plugin values,
  and `#src/*` resolves under both conditions (source under --conditions=typestrip,
  dist otherwise) — the extensionless relative require only resolved against dist,
  which was a one-entry regression from the previous static `.ts` import. Also drops
  the getter's overclaiming comment: defaultConfig ships `modelsGateway.enabled:
  false`, so the config block is present and the module loads regardless.
- Symmetry: `/v1/embeddings` never got the rejected-body-promise guard its sibling
  received, so malformed JSON there still escaped as an RFC 9457 500. Same try/catch
  as chatCompletions.
- Enabling the gateway by its own documented example produced a silently unservable
  gateway (defaultConfig ships no `rest` section → every /v1 path 404s with no
  diagnostic). The example now shows `rest: true`, and handleApplication warns when
  enabled with no `rest`/`REST` configured.
- OpenAI content parts (`content: [{type:'text',text}]`) passed through unvalidated
  into a `Message.content` declared as string. Text parts are now flattened and
  unsupported part types return a 400.
- The tool-argument bound counted all accumulated keys per delta, so the O(n^2) the
  Object.assign change was meant to remove was still there in the check. Now counts
  only newly-introduced keys, and ignores a contract-violating string `arguments`
  rather than counting its characters. Bounds now have tests, including the
  at-the-cap off-by-one.
- connect(): the docstring's premise was wrong — the pre-override behavior was
  Resource's default connect returning an empty subscription (a forever-open SSE),
  not method-not-allowed. Corrected, and the WebSocket call path (no request.data)
  now returns a client error instead of an envelope that path cannot iterate.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
Comment thread components/componentLoader.ts
Comment thread integrationTests/server/v1-gateway.test.ts Outdated
@heskew
heskew requested a review from kriszyp July 24, 2026 05:13
…t auto-started

The header still described the removed REST.ensureStarted auto-activation; the
suite now declares an explicit `rest` section like a real deployment. (Bot review.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
heskew added a commit that referenced this pull request Jul 24, 2026
Bring the LangChain e2e + encoding_format work current with the reworked
gateway before folding it into #1616.

# Conflicts:
#	package.json
#	unitTests/resources/models/v1/embeddings.test.js
Comment thread components/componentLoader.ts Outdated
// REST with default options here, after every root plugin has loaded: if any
// `rest`/`REST` section exists — whatever its key order — its handleApplication
// already ran with the user's options and this is a no-op.
if (isRoot && resources.isWorker && (config as any).modelsGateway?.enabled) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid starting the singleton, unscoped REST router here? Root components load before application directories, so this makes every REST-exportable entry in the shared Resources registry reachable even for an app that intentionally omitted rest. It also sets REST.started before a later app can register its configured port/host/urlPath/WebSocket options. A gateway-specific /v1 handler (or independent scoped REST registration) would keep this opt-in from changing unrelated application exposure; please add a mixed-app regression test as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both halves addressed, but the test turned up something about the framing worth your read.

The force-start is gone. 6e8cbc7 removed it; the gateway is a plain plugin that requires REST to already be configured, and warns rather than starting it. 42c9bb5 went further and dropped the modelsGateway block from defaultConfig.yaml, so with the key absent the root loop skips the component before resolving it — an instance that hasn't opted in doesn't even import the module graph.

Mixed-app regression test added in fcab43d (integrationTests/server/v1-gateway-mixed-app.test.ts + fixture app that @exports a table and declares no rest).

The finding: the exposure you described isn't gateway-specific. Once any root rest section exists, REST serves the shared Resources registry, and that app's @exported table is reachable whether or not the app declared rest itself. I checked this empirically — my first version of the test asserted the table would be unreachable with rest configured, and it returned 200.

So enabling the gateway is not what makes a non-rest app reachable; configuring REST at all is. The gateway's own contribution is only observable with no rest anywhere, which is exactly what the old force-start manufactured — and that is how the primary suite is configured: gateway enabled, no rest, assert the app stays off the wire.

Whether REST ought to be scoped per-application rather than serving the shared registry is the real question underneath your comment, and it's the same one tracked in #1931. This PR doesn't change that behavior in either direction; the test pins that it doesn't.

The file also carries a control suite that runs the identical fixture and URL with rest configured and asserts the table IS reachable. Without it the primary assertions could pass vacuously — a typo'd path or a fixture that never deployed would also look "not reachable". If the guard ever goes hollow the control fails loudly.


🤖 Posted by Claude on Nathan's behalf

Comment thread resources/models/v1/translation.ts
Comment thread resources/models/v1/translation.ts Outdated
Comment thread resources/models/v1/chatCompletions.ts Outdated
Comment thread resources/models/v1/embeddings.ts Outdated
Comment thread resources/models/v1/chatCompletions.ts
…nabled

The gateway is registered in componentLoader's built-in table, and the root
loader's only pre-resolution guard is `if (!componentConfig) continue`. A
shipped `modelsGateway: { enabled: false }` block is a truthy object, so every
instance resolved the entry and imported the whole /v1 module graph on every
startup and every worker — handleApplication then returned immediately on its
`enabled` check, having already paid for the import.

Dropping the block from defaultConfig.yaml makes "off" genuinely free: with the
key absent the loader skips the component before resolving it. Nothing is
stranded — #1616 has not merged, so no install has ever carried the key, and
CONFIG_PARAM_MAP is populated from the CONFIG_PARAMS enum rather than from
defaultConfig.yaml, so removing it orphans nothing.

The shipped config is not a discovery surface — docs are — but the ops API
should still work. modelsGateway was absent from CONFIG_PARAMS, so
set_configuration rejected `modelsGateway_enabled` as an unrecognized config
parameter, leaving hand-editing YAML as the only way to enable the gateway.
Registering MODELSGATEWAY_ENABLED matches localStudio (LOCALSTUDIO_ENABLED),
its closest analogue.

packaging.test.js pins both invariants so neither is silently undone.

Addresses @kriszyp's zero-cost-when-disabled review thread on #1616.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014fE3ARcG11oxJXRy3SvWYG
Comment thread unitTests/resources/models/v1/embeddings.test.js
heskew and others added 2 commits July 28, 2026 06:22
…dings

chatCompletions covered the rejecting body promise; embeddings had the same
guard but no test pinning it. Closes the asymmetry noted on the review thread.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014fE3ARcG11oxJXRy3SvWYG
…havior

Pins the property the removed force-start was traded for: enabling modelsGateway
must not stand REST up on another component's behalf.

The fixture app exports a table but declares no `rest` section. Finding worth
recording: once a root `rest` section exists, REST serves the shared Resources
registry and that table is reachable regardless of the app's own `rest` — that
is existing Harper behavior and not caused by the gateway. So the gateway's
contribution is only observable with no `rest` configured anywhere, which is how
the primary suite is set up. Previously, enabling the gateway alone was enough
to start REST and expose the app.

A control suite runs the identical fixture and URL with `rest` configured and
asserts the table IS reachable. Without it the primary assertions could pass
vacuously — a typo'd path or an undeployed fixture would also look "not
reachable". The control fails loudly if the guard ever goes hollow.

Also corrects two comments in v1-gateway.test.ts that said the gateway is off by
default via `enabled: false` in defaultConfig.yaml; since 42c9bb5 the block is
absent entirely, which is what makes the disabled path cost nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014fE3ARcG11oxJXRy3SvWYG
@heskew
heskew requested a review from kriszyp July 28, 2026 15:11

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress, a few more things to cover.
🤖 Reviewed with GPT 5.6

Comment thread resources/models/v1/index.ts
Comment thread resources/models/v1/index.ts Outdated
Comment thread resources/models/v1/translation.ts
Comment thread resources/models/v1/translation.ts
Comment thread resources/models/v1/errors.ts
Comment thread resources/models/v1/embeddings.ts Outdated
Comment thread package-lock.json Outdated
Comment thread resources/models/v1/index.ts Outdated
Comment thread resources/models/openaiStream.ts Outdated
heskew and others added 10 commits July 30, 2026 18:23
…ration

An earlier lockfile regeneration (npm 10, from the #1856 branch work) flipped
all six @msgpackr-extract/* platform entries to dev:true — which would make
build-tools/prune-shrinkwrap-dev.mjs delete the host msgpackr binary from the
published shrinkwrap — and dropped the libc selectors from the four
@harperfast/rocksdb-js-linux-* optionals, which makes npm on Linux install
both the glibc and musl variants (verified empirically in a node:24.18.0
container: dry-run and real `npm ci --omit=dev` both selected two variants).

Fix: regenerated with the canonical toolchain (Node 24.18.0 per .node-version,
full install so real manifests are read), which heals the dev flags, then
restored the four libc fields verbatim from main's identical 2.5.0 entries.
The libc restore has to be verbatim-from-main because NO npm install path can
produce it today: the registry's abbreviated packument omits `libc` entirely
(only cpu/dist/engines/name/os/version), so any re-resolution of a bumped
version silently drops it — which is exactly how this regressed when 2.4.0
became 2.5.0. (That also means the next rocksdb bump on main will hit the
same trap; flagged on the PR.)

Verified in a pristine node:24.18.0-bookworm container:
- `npm ci --omit=dev` (dry-run and real) selects exactly one libc variant
- @msgpackr-extract binaries survive --omit=dev on disk
- prune-shrinkwrap-dev.mjs retains all 6 msgpackr entries and all 4 libc fields
- a follow-up canonical `npm install --package-lock-only` leaves the lock
  byte-stable (no re-resolution, metadata preserved)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
…emoved

Deleting a component's block from the config emits `remove` from
OptionsWatcher, but Scope only wired `change` — so a running component kept
serving until some unrelated restart. That gap became user-visible with the
/v1 models gateway, where an absent block is the canonical disabled state
(@kriszyp's review): deleting `modelsGateway:` left the three routes live.

Fixed generically in Scope rather than per-plugin: a `remove` listener that
mirrors the change listener's contract — if the plugin registers its own
`remove` handler it owns the response; otherwise Scope requests a restart.
Regression tests cover both the restart-on-deletion path and the
plugin-owned-removal path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
…placement

Resources.set identifies collisions by databaseName/tableName, which are both
undefined for non-table Resources — so an application class with
`static path = 'v1/models'` silently replaced the gateway (and its super_user
gate) with whatever auth the app class carries. Reviewed as a route-hijack
risk on PR #1616.

Fix: a `static reservedPath = true` marker on the three gateway Resources;
Resources.set now treats a differing-class registration against a reserved
path as a conflict (existing behavior: ErrorResource at that path + logged
error), while same-class re-registration stays idempotent and `force` still
overrides. Plain app-vs-app replacement semantics are unchanged.

Verified live via direct spawn: contested v1/models returns a 500 conflict
envelope (never the imposter payload), uncontested gateway routes and the
app's own routes serve normally. Also covered by a new collision fixture in
the mixed-app integration suite and unit tests over set/getMatch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
…nors blocks

The gateway registrations carried no exportTypes policy, so the shared
registry matched them for every protocol lookup — WS dispatch could reach
V1ChatCompletions.connect(), get the non-iterable badRequest envelope, and
fail async-iterating it; the endpoints also surfaced through MQTT/GraphQL/MCP
enumeration. Reviewed on PR #1616.

Registration now passes REST-only visibility for all three endpoints plus SSE
for chat/completions (explicit Accept: text/event-stream dispatch).

Writing the routing assertions exposed a latent Resources.getMatch bug: the
exportTypes check only guarded the relativeURL assignment on the exact-path
and root-fallback paths, so a protocol-blocked entry was still returned to
the caller (MCP worked around this with its own isMcpExposed re-check).
Blocked entries are now treated as not-found and fall through to
paramRoutes/root like any other miss.

Covered by a new routing unit suite (REST serves all three, SSE chat-only,
ws/mqtt/graphql/mcp invisible); full resources + components + mcp unit suites
pass unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
…noring them

validateChatRequest only checked messages/tools shape, so malformed top-level
fields slipped through: a non-string model silently ran the configured
default backend, a truthy non-boolean stream (e.g. the JSON string "false")
returned SSE the client didn't ask for, and malformed temperature/max_tokens
values were forwarded to backends to fail in provider-specific ways.
Reviewed on PR #1616.

Now rejected up front with OpenAI invalid_request_error envelopes: non-string
model (mirrored on /v1/embeddings), non-boolean stream, non-finite or
out-of-range temperature (0–2 per OpenAI), and non-positive-integer
max_tokens / max_completion_tokens.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
…ed 500s

ModelCapabilityError extends ServerError, so toOpenAIError's statusCode
branch treated it as a genuine backend failure: logged, sanitized to a
generic 'Internal server error' 500. But a capability mismatch is
caller-driven — sending `tools` (or requesting streaming) to a configured
backend whose capabilities say otherwise — and the client needs to see what
to change. Reviewed on PR #1616.

Now mapped to 400 invalid_request_error with code capability_unsupported,
message passed through (it names only the backend and the capability asked
for). Covers both response paths: the JSON envelope and the SSE error frame
share this mapping via formatError. Genuine 5xx sanitization is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
…n roles

validateChatRequest accepted any string role and translateMessages cast it
into Harper's Message union, so 'developer' — a current Chat Completions
role — passed through untyped and provider adapters degraded it to 'user',
silently demoting the instruction's priority. Reviewed on PR #1616.

'developer' is OpenAI's successor to 'system' (their API treats the two
identically, converting system to developer on newer models), and every
provider adapter maps Harper's 'system' to its system-instruction slot — so
the gateway now normalizes developer → system at translation, preserving the
channel across all backends without widening the internal Message union that
custom backends implement. Roles outside
system/developer/user/assistant/tool are now a 400 (including the deprecated
'function' role) instead of a silent cast.

Covered on both paths kris named: pure translation (developer → system) and
a composed gateway→AnthropicBackend test asserting the developer message
lands in Anthropic's top-level system field, not a user turn.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
Every successful embeddings response reported zero usage: models.embed()
unwraps ModelCallResult and drops the usage all built-in embedding backends
already provide, and the gateway passed nothing to toEmbedResponse.
Reviewed on PR #1616.

Models gains embedWithUsage() — embed() plus the winning backend's
result-level usage, with embed() now delegating to it — as the internal
gateway path kris prescribed; the public embed() contract is unchanged and
the analytics/fallback behavior is shared, not duplicated. The gateway
passes that usage through, and toEmbedResponse now maps embeddingTokens
(promptTokens fallback) into BOTH prompt_tokens and total_tokens — the
previous asymmetric mapping left prompt_tokens at 0 for backends that only
report embeddingTokens, which is all of the built-ins.

Covered at three layers: embedWithUsage preserves usage on the same
analytics path, the wire mapping is symmetric, and a gateway post() test
asserts nonzero real counts end-to-end through TestBackend.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
The call-count and key-count caps didn't bound memory: a single argument key
can retain an arbitrarily large value from a custom backend, and the final
JSON.stringify duplicates the retained allocation — up to 256 calls of
amplification on a public HTTP path. Reviewed on PR #1616.

Assembly now charges one cumulative per-stream budget (1,048,576 serialized
chars) covering call ids, names, and every argument value as it arrives.
Charging is per delta — no re-serialization of the accumulator, keeping the
O(delta) accounting — and monotonic: replacing an existing key charges the
new value too, so churn under a stable key count cannot smuggle unbounded
values past the bound. Overflow terminates through the existing
ToolAssemblyOverflowError sanitized SSE error-frame path.

Tests: one oversized single value (past the budget, under both count caps),
replacement churn on one key, and an under-budget large value completing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
Cross-model review of 88af257 caught the counter under-approximating the
final serialization: it charged raw key chars and serialized values but not
the quotes/colon/comma each entry contributes (~4 chars) or the flush
frame's fixed per-call envelope, so a flood of tiny entries could reach
~3x the nominal budget in real serialized size. First adds now charge
key + 4 and call creation charges id + 96, making the cumulative count an
upper bound on JSON.stringify of the retained arguments (the SSE frame
adds only bounded escaping on top). Replacement charges are still never
refunded, preserving the monotonic O(delta) accounting.

Test: a 150-call x 1024-tiny-key flood whose raw chars fit the budget but
whose serialized JSON exceeds it now trips, under both count caps.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
@heskew
heskew requested a review from kriszyp July 31, 2026 16:31
CI caught the shared-buffer test failing after cfb3b5c: the buffer is
process-wide, and Scope's new remove→restart behavior means earlier suites
now legitimately set it — componentLoader.test.js fixtures leave scopes
open whose watched harperdb-config.yaml is deleted at teardown, and
chokidar's delayed unlink timer delivers the remove (correctly requesting a
restart) several tests later. The test's entry assertion encoded 'no prior
test ever requested a restart', an ordering assumption, not this module's
contract. Reset at entry — the same convention Scope.test.js already uses —
so the test pins the false→true transition it was written for.

(Also corrects an earlier triage note: this failure was called pre-existing
based on a stash control run against a stale dist build; with a rebuilt
control it is attributable to the Scope change's interaction with unclosed
fixture scopes, which this makes order-robust.)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXx78W5LxbvxEdSyrB1vvn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Models] Phase 4 — OpenAI-compatible /v1/* gateway as built-in Resources

2 participants